Skip to content

sqlite: bind undefined to NULL - #65709

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite-bind-undefined-to-null
Sep 8, 2026
Merged

nodejs-github-bot merged 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite-bind-undefined-to-null

Conversation

@TrevorBurnham

@TrevorBurnham TrevorBurnham commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #61824

This PR re-implements #62008 by @mike-git374. The one-line behavior change is theirs; this PR adds the documentation and the remaining test coverage.

Omitting a named parameter binds NULL, but passing undefined for that same parameter threw ERR_INVALID_ARG_TYPE:

s.run({ k: 1 });               // v binds to NULL
s.run({ k: 2, v: undefined }); // throws ERR_INVALID_ARG_TYPE

Binding undefined to NULL makes those agree. It also matches JSValueToSQLiteResult, which already maps a user-defined function's undefined return value to NULL, and SQLite's own WASM oo1 API.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Sep 1, 2026
Omitting a named parameter binds NULL, but passing `undefined` for that
same parameter threw ERR_INVALID_ARG_TYPE. Bind `undefined` to NULL so
the two forms agree. This matches the conversion already applied to a
user-defined function's `undefined` return value, as well as SQLite's
own WASM oo1 API.

Fixes: nodejs#61824
Refs: nodejs#61472
Refs: nodejs#62008
Co-authored-by: mike-git374 <217764531+mike-git374@users.noreply.github.com>
Assisted-by: claude:opus-5
Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
@TrevorBurnham
TrevorBurnham force-pushed the sqlite-bind-undefined-to-null branch from 5cb500c to 47bbca8 Compare September 1, 2026 15:45
@TrevorBurnham
TrevorBurnham marked this pull request as ready for review September 1, 2026 15:51
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.04%. Comparing base (f9ab994) to head (47bbca8).
⚠️ Report is 79 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65709      +/-   ##
==========================================
- Coverage   90.07%   90.04%   -0.03%     
==========================================
  Files         754      754              
  Lines      256395   256395              
  Branches    48494    48499       +5     
==========================================
- Hits       230947   230875      -72     
- Misses      16563    16639      +76     
+ Partials     8885     8881       -4     
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.05% <100.00%> (+0.06%) ⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@trivikr trivikr added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 4, 2026
@trivikr
trivikr requested a review from Qard September 4, 2026 04:40
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 4, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@trivikr trivikr added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 4, 2026
@nodejs-github-bot nodejs-github-bot added the lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. label Sep 4, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 41cb09f into nodejs:main Sep 8, 2026
101 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 41cb09f

@nodejs-github-bot nodejs-github-bot removed commit-queue PRs queued for automated landing through the Commit Queue. lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. labels Sep 8, 2026
aduh95 pushed a commit that referenced this pull request Sep 16, 2026
Omitting a named parameter binds NULL, but passing `undefined` for that
same parameter threw ERR_INVALID_ARG_TYPE. Bind `undefined` to NULL so
the two forms agree. This matches the conversion already applied to a
user-defined function's `undefined` return value, as well as SQLite's
own WASM oo1 API.

Fixes: #61824
Refs: #61472
Refs: #62008
Co-authored-by: mike-git374 <217764531+mike-git374@users.noreply.github.com>
Assisted-by: claude:opus-5
Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
PR-URL: #65709
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 16, 2026
mceachen added a commit to photostructure/node-sqlite that referenced this pull request Sep 18, 2026
Passing undefined for a parameter threw while omitting the same named
parameter bound NULL, so the two forms disagreed. This also forced
callers using query builders like Knex, which emit undefined for columns
missing from a multi-row insert, to sanitize bindings by hand.

Ports nodejs/node#65709.
nodejs-github-bot added a commit that referenced this pull request Sep 20, 2026
Notable changes:

crypto:
  * (SEMVER-MINOR) add crypto.parsePKCS12() (Brian Muenzenmeyer) #65627
doc:
  * add araujogui to collaborators (Guilherme Araújo) #66090
ffi:
  * (SEMVER-MINOR) load libraries from a mounted VFS (Matteo Collina) #65909
fs:
  * (SEMVER-MINOR) add openAsBlobSync (greenhead) #65644
net:
  * (SEMVER-MINOR) support sending net.BoundSocket to threads and child processes (Guy Bedford) #64725
perf_hooks:
  * (SEMVER-MINOR) implement SlidingWindowHistogram (James M Snell) #65825
  * (SEMVER-MINOR) implement qrde analysis support in Histogram (James M Snell) #65806
sqlite:
  * (SEMVER-MINOR) bind undefined to NULL (Trevor Burnham) #65709
src,lib:
  * (SEMVER-MINOR) add util.markPromiseAsHandled (James M Snell) #65805
test:
  * (SEMVER-MINOR) expand histogram test coverage (James M Snell) #65825
util:
  * (SEMVER-MINOR) implement util.throttle (James M Snell) #65899
  * (SEMVER-MINOR) implement debounce (James M Snell) #65899

PR-URL: #66163
aduh95 pushed a commit that referenced this pull request Sep 22, 2026
Notable changes:

crypto:
  * (SEMVER-MINOR) add crypto.parsePKCS12() (Brian Muenzenmeyer) #65627
doc:
  * add araujogui to collaborators (Guilherme Araújo) #66090
ffi:
  * (SEMVER-MINOR) load libraries from a mounted VFS (Matteo Collina) #65909
fs:
  * (SEMVER-MINOR) add openAsBlobSync (greenhead) #65644
net:
  * (SEMVER-MINOR) support sending net.BoundSocket to threads and child processes (Guy Bedford) #64725
perf_hooks:
  * (SEMVER-MINOR) implement SlidingWindowHistogram (James M Snell) #65825
  * (SEMVER-MINOR) implement qrde analysis support in Histogram (James M Snell) #65806
sqlite:
  * (SEMVER-MINOR) bind undefined to NULL (Trevor Burnham) #65709
src,lib:
  * (SEMVER-MINOR) add util.markPromiseAsHandled (James M Snell) #65805
test:
  * (SEMVER-MINOR) expand histogram test coverage (James M Snell) #65825
util:
  * (SEMVER-MINOR) implement util.throttle (James M Snell) #65899
  * (SEMVER-MINOR) implement debounce (James M Snell) #65899

PR-URL: #66163
JumpLink added a commit to gjsify/gjsify that referenced this pull request Sep 22, 2026
* fix(sqlite): undefined binds NULL, like Node 26.10

node:sqlite refused an explicitly-passed `undefined` until v26.10.0, where
nodejs/node#65709 made it bind NULL so that passing a parameter as `undefined`
agrees with omitting it — which bound NULL already, on both implementations.
The release documented the rule in doc/api/sqlite.md and dropped `undefined`
from the "unsupported data types" list this spec is ported from.

`@gjsify/sqlite` carried exactly the asymmetry Node removed: validateBindValue()
returned early for `null` alone, while sqlLiteral() below it already rendered
`undefined` as NULL and an omitted parameter already bound NULL. It now accepts
`undefined` on both legs, unconditionally.

`undefined` leaves the unsupported list but is not deleted: it moves into a test
of its own that asserts the stored value reads back `null` and that an explicit
`undefined` and an absent argument land on the same value. The Node leg runs
native node:sqlite (runtimes.node is "none") and the hosts disagree across the
26.10 line, so the boundary is named rather than left silent.

* test(sqlite): which sqlite, not which host

The `undefined` gate asked `process.versions`, so it answered "which host am I
on" — but `test:gjs-on-node` builds these same specs with
`--alias node:sqlite=@gjsify/sqlite` and runs OUR implementation on a Node host.
Host and implementation come apart there, and only the implementation decides
what `stmt.run(1, undefined)` does. Measured, one probe per leg:

  node-gi (aliased)  host_builtin=false  node=24.19.0  gjs=undefined
  native node        host_builtin=true   node=24.19.0  gjs=undefined
  gjs                host_builtin=false  node=20.0.0   gjs=1.88.1

Row 1 is the red: Node 24 reads as "refuses", so the test demanded a throw from
an implementation that binds NULL — `Expected [anonymous function] to throw an
exception`, on "sqlite suite as node-gi consumer" and "node-gi consumer
harness". Row 3 is the trap the old gate already knew about and patched with an
`IS_GJS` check; rows 1 and 3 are the same defect, and Bun and Deno would be two
more.

So the gate asks the module instead. `[native code]` is what ECMA-262 renders
for a function with no ECMAScript source: true means the binding came from the
host, false means it came from a bundle of our TypeScript. The version is read
only after that answer, and `IS_GJS` is gone because it is subsumed. It cannot
mask a regression in the direction that matters — were `@gjsify/sqlite` to go
back to refusing `undefined`, the probe stays false and the test still demands
NULL. Verified by breaking `validateBindValue` back to `value === null`: gjs
89/90 and node-gi 64/65, both on `InvalidArgTypeError: Provided value cannot be
bound to SQLite parameter 2.`

Green after: gjs 90 pass / 178 assert, node 24.19.0 90 / 176,
node-gi on node 24.19.0 65 / 135.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sqlite: inconsistent undefined bind to null

4 participants